Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialise locale formatting in advance as a performance improvement #179

Merged
merged 7 commits into from
May 8, 2024

Conversation

tunetheweb
Copy link
Member

@tunetheweb tunetheweb commented May 7, 2024

Issue raised in #178 (comment)

This initialised the formatters once as the extension is initialised, rather than with each metric update for a god performance improvement, but also allows us to keep the INTL option.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's probably a good argument for designing this in such a way that breaks up and defers possible long tasks to idle time...but I'm on board with not overcomplicating it and erring strongly on the side of the extension not interfering with page performance

src/browser_action/metric.js Outdated Show resolved Hide resolved
src/browser_action/vitals.js Outdated Show resolved Hide resolved
@tunetheweb
Copy link
Member Author

@brendankenny upon suggestion from @rviscomi I'm initialising the three formatters needed up front once, rather than each time the metric changes.

As the extension is only initialised after the document is complete there's a bigger chance it's idle then (not guaranteed, but best chance) so initialising the NumberFormat then (and only then) is best place to init it.

WDYT?

@tunetheweb tunetheweb changed the title Avoid localestring as known to cause performance issues Avoid localestring per metric performance improvement May 8, 2024
@tunetheweb tunetheweb changed the title Avoid localestring per metric performance improvement Avoid locale formatting per metric update as a performance improvement May 8, 2024
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Relatively straightforward, and if we do somehow detect there's some longer tasks when the script is injected we can always revisit with idle detection or whatever.

@tunetheweb tunetheweb merged commit be694b7 into main May 8, 2024
3 checks passed
@tunetheweb tunetheweb changed the title Avoid locale formatting per metric update as a performance improvement Initialise locale formatting in advance as a performance improvement May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants